Convert RSA envelope encryption to JWE #767
Conversation
SgtCoDFish
force-pushed
the
jwe
branch
2 times, most recently
from
9a85e11 to
8b85fa9
Compare
SgtCoDFish
force-pushed
the
jwe
branch
2 times, most recently
from
41454cb to
00389ff
Compare
| defer func() { | ||
| for i := range aesKey { | ||
| aesKey[i] = 0 | ||
| } | ||
| }() |
There was a problem hiding this comment.
note: we could generate the key outside of jwe.Encrypt and pass it in using WithKey, and keep our zeroising function here. Since we have to pass the key into a third party library in that case, I don't think we really gain much by zeroising here (we can't control what that third party does when the library updates).
This is what go1.26's secret.Do will be for. For now, having the jwe library generate the key means we never have to touch it in our code in plaintext.
Uses github.com/lestrrat-go/jwx/v3, should be the same functionality as before Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
| github.com/google/uuid v1.6.0 | ||
| github.com/hashicorp/go-multierror v1.1.1 | ||
| github.com/jetstack/venafi-connection-lib v0.5.2 | ||
| github.com/lestrrat-go/jwx/v3 v3.0.13 |
There was a problem hiding this comment.
Looks like a single-person project (99% of commits come from them), but I guess I'm OK with the risk
There was a problem hiding this comment.
It's a fair comment - I liked that there were plenty of external contributors even though they weren't contributing lots of code. I don't feel scared by it, personally!
| encrypted, err := jwe.Encrypt( | ||
| data, | ||
| jwe.WithKey(jwa.RSA_OAEP_256(), e.publicKey, jwe.WithPerRecipientHeaders(headers)), | ||
| jwe.WithContentEncryption(jwa.A256GCM()), | ||
| jwe.WithCompact(), |
There was a problem hiding this comment.
From https://github.com/jetstack/jetstack-secure/pull/767/changes#r2753707291:
note: we could generate the key outside of
jwe.Encryptand pass it in usingWithKey, and keep our zeroising function here. Since we have to pass the key into a third party library in that case, I don't think we really gain much by zeroising here (we can't control what that third party does when the library updates). This is what go1.26's secret.Do will be for. For now, having the jwe library generate the key means we never have to touch it in our code in plaintext.
That makes sense. Is the memory zeroing an important concern? I've looked at the https://github.com/lestrrat-go/jwx lib and wasn't able to find mentions to zeroing, seems like it does, https://github.com/lestrrat-go/jwx/blob/1f715710fa563331b213e2dc81565e9f5e36ab2e/jwk/rsa.go#L105 suggests that it does but haven't dug.
There was a problem hiding this comment.
Honestly I think the incoming secret.Do shows that zeroing is mostly a bit of theater ATM. We need language level support (like secret.Do) to be confident that we're doing zeroing, or it's just a bit too error prone and there's too much scope for things to get copied and not zeroed.
It's probably a solvable problem but I don't think it's worthwhile to spend that time!
| return nil, fmt.Errorf("failed to generate nonce: %w", err) | ||
| // Create headers with the key ID | ||
| headers := jwe.NewHeaders() | ||
| if err := headers.Set("kid", e.keyID); err != nil { |
There was a problem hiding this comment.
nit: Worth adding a test case checking that the kid is correctly set in the JWE header? maybe not a big deal
|
Looks good to me. I haven't dug into the cryptographic side of things. My only remark was that we are relying on RSA key signature but not a big deal. While reviewing, I've discovered an interesting CLI named I wish I had been able to test the new library using |
This matches what the upstream will expect - we agreed last week to use JWE in this case. To make it easier to migrate to other encodings later, I make the EncryptedData struct depend on a tag.
Important: This code still remains unused! The aim is to get the cryptography correct for now - we'll use it later.
Uses the github.com/lestrrat-go/jwx/v3 library, which seemed like the best choice from the ones I surveyed (e.g. https://github.com/golang-jwt/jwe has 2 contributors and no commits for 4 years)